Repository navigation
docs: document the experimental Cassandra storage backend - #412
jcshepherd wants to merge 1 commit into
Conversation
LeeroyHannigan
left a comment
LeeroyHannigan
left a comment
There was a problem hiding this comment.
I built this branch (5a4a0a6) with --features cassandra and followed the setup guide against cassandra:4.1 (4.1.12) in Docker.
The guide doesn't get a reader to a running server. init fails on a node without authentication, and on an authenticated node serve can't connect with the config init writes. The causes are in init and the Cassandra connection code, and those fixes don't belong in this PR. The six inline comments ask the docs to describe what the backend does today. Once they're addressed, this is good to merge from my side.
Not measured: a multi-node cluster and Cassandra 5.x. The replication factor note is from the code.
LeeroyHannigan
left a comment
LeeroyHannigan
left a comment
There was a problem hiding this comment.
I re-checked the six comments from 22 October against a549428 and read the changed docs against origin/main. Four are addressed and correct: the PasswordAuthenticator requirement, the AllowAllAuthorizer caveat, the datacenter1 and replication_factor notes, and the unencrypted connection. Two need another pass. The credentials text is now stale: #384 changed init to write username and password live, so my earlier comment on that point predates the change. The Streams sentence states the Cassandra exception too narrowly and leaves MongoDB out. Three optional points I missed in the first round are marked as such and are not merge conditions. Once the two are fixed I will approve. Not run: init or serve against a live node.
There was a problem hiding this comment.
I re-checked the two items from round 2 against 183903b and ran both guides on a fresh cassandra:4.1 node. Serve started with the config init wrote, without edits, and CreateTable, PutItem and GetItem worked.
The Streams paragraph matches the code on all four backends now, and MongoDB is in.
The credentials text is fixed in getting-started.md but not in cassandra-setup.md, which still says init writes them commented out and still has the block telling the reader to uncomment them. The new sample lines in both files have a problem of their own, see the inline comments.
The renumbered ADR headings match their file names, and the docs references point at the right ADRs.
I've left three optional comments on things we should have caught in the earlier rounds, they aren't merge conditions. Separately, Cassandra's TransactWriteItems doesn't write stream records (a transaction applied and produced none), and extenddb destroy can't log in to an authenticated node. Both are on main and already filed, as #516 and #500.
Once the credentials text and the two samples are fixed I'll approve.
|
|
||
| [storage.cassandra] | ||
| contact_points = ["127.0.0.1:9042"] | ||
| username = <cassandra-user> # From extenddb init args |
There was a problem hiding this comment.
These don't parse. With these two lines in the config, the server stops with TOML parse error at line 30, column 12 ... invalid string. They also point at the wrong flags. Init writes the application role from --extenddb-user and --extenddb-pass, the --cassandra-* admin flags never reach the file. With --extenddb-user app2 --extenddb-pass app2-pass the file had "app2" and "app2-pass". Something like:
username = "extenddb" # --extenddb-user, default extenddb
password = "extenddb-local-dev" # --extenddb-pass, default extenddb-local-dev| ```toml | ||
| [storage.cassandra] | ||
| contact_points = ["127.0.0.1:9042"] | ||
| username = <cassandra-user> # From extenddb init args |
There was a problem hiding this comment.
Same problem as cassandra-setup.md line 143, these need to be the quoted application role values.
There was a problem hiding this comment.
So, if I make the suggested change, than I can virtually guarantee the agent will come back with "You have plain-text credentials in your document" and/or "This will break if the user ran init with other credentials". There is no winning. Also, this notation is used elsewhere in our documentation for non-Cassandra use-cases. It might be confusing to an agent (maybe?) but not to a human. This isn't useful feedback.
|
At some point the agent reviewing this should be asking itself if it has crossed the threshold where the time/effort to be spent on yet another iteration of refinements, merges from main, etc., are more valuable than making the documentation with minor imperfections, available to users so they can actually use the damn product. |
|
Joel, point taken. Three rounds on a docs PR is too many and the later ones added things the first should have caught. I've resolved 18 of the 19 threads, including the two sample-line comments, the placeholder notation is the same one the rest of the docs use. The one thread still open is cassandra-setup.md line 135. That paragraph says init writes the credentials commented out and tells the reader to uncomment them, and init has written them live since #384. Fix that paragraph and I'll approve. Nothing else gets added. The three optional items from round 3 are in #658 so they don't get lost. They aren't conditions on this PR. |
* Correct numbering in Cassandra-related ADRs * Remove PostgreSQL-specific references in affected documents where a statement applies generally to all backends * Added Cassandra-specific setup and getting-started directions * Document many known differences and gaps in the Cassandra backend
bbe204c to
54b83a9
Compare
|
Removed stale references to editing extenddb.toml from the cassandra-setup.md documentation; merged from main and squashed. |
|
Joel, the credentials paragraph is fixed, thanks. I resolved the two sample-line threads this morning and I shouldn't have. You're right about the notation, so keep the placeholders. But The architecture guide's Streams port sentence is in #658, not a condition here. |
What
Updates existing documentation to include the Apache Cassandra backend (marked as experimental throughout), adding one new document on Cassandra set-up.
I did make a gentle attempt to make the existing documentation less Postgres-centric where appropriate, primarily by eliding Postgres references when it didn't change the meaning of the section. There is still more work to be done here, as well as some technical issues to fix (e.g., default Postgres database names), but those should be handled in separate changes.
This is not intended to be a broad documentation rewrite.
Why
Bring user-facing documentation for the Cassandra backend up to par with the other backends.
Testing done
Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.